Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

populate dimensions for restored json unit registries from dimensions namespace #81

Merged
merged 1 commit into from
Mar 27, 2019

Conversation

ngoldbaum
Copy link
Member

We use "is" equality in a bunch of places when comparing dimensions. This is problematic given the current approach for the json saving and loading functionality, since sympify will generate distinct Symbol objects. We could replace the is equality uses with ==, but that is anywhere from a few times to 50 times slower:

In [13]: from unyt.dimensions import length, energy

In [14]: %timeit length is length
34.8 ns ± 0.326 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [15]: %timeit length == length
128 ns ± 6.87 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [16]: %timeit length is not energy
39.8 ns ± 5.37 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [17]: %timeit length == energy
2.38 µs ± 155 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [18]: 128/34.8
Out[18]: 3.6781609195402303

In [19]: 2380/39.8
Out[19]: 59.798994974874375

The solution for the particular instance of this issue we're running into here is to use the locals keyword argument for sympify, which gives it access to a namespace of objects to substitute in expressions, which allows us to use the dimensions in unyt.dimensions to populate the dimensions of the restored unit registries.

@ngoldbaum ngoldbaum merged commit 66c7502 into yt-project:master Mar 27, 2019
@ngoldbaum ngoldbaum mentioned this pull request May 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant